Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basic support for new-in-initializer #238

Open
wants to merge 1 commit into
base: 1.23.x
Choose a base branch
from

Conversation

rvanvelzen
Copy link
Contributor

Implementation for #173

@ondrejmirtes
Copy link
Member

Do you see this somehow being useful for the analysis, or is it just because we don't have a different syntax for "let's make this parameter optional but not nullable"?

One thing this will impact are types resolved for generics and conditional types (that should be tested), but otherwise not much imho.

@rvanvelzen
Copy link
Contributor Author

Could you explain what this has to do with optional or non-nullable parameters?

One thing this will impact are types resolved for generics and conditional types (that should be tested), but otherwise not much imho.

While this is true, for default values in regular code generics aren't taken into account either. So implementing this in PHPStan itself is just a few lines of code.

@ondrejmirtes
Copy link
Member

They are: https://phpstan.org/r/fe352005-91ed-4021-bd95-9d6df9f886a4 (and not generalized on bleeding edge https://phpstan.org/r/7fad672b-f8b0-4128-9282-d82a6599f2b6, unless in GenericObjectType).

@ondrejmirtes
Copy link
Member

Could you explain what this has to do with optional or non-nullable parameters?

I don't really get the question, because the answer is "everything"? This syntax applies only to default value of parameters in @method. So I was interested how this enhances analysis in PHPStan. And the answer to that is that besides marking the parameter as optional, it will influence the result of generics and conditional types. Right?

@rvanvelzen
Copy link
Contributor Author

They are: phpstan.org/r/fe352005-91ed-4021-bd95-9d6df9f886a4 (and not generalized on bleeding edge phpstan.org/r/7fad672b-f8b0-4128-9282-d82a6599f2b6, unless in GenericObjectType).

Ah, I actually meant that the default value itself is never generic: https://phpstan.org/r/dc1fee3f-b8de-448d-ba24-edc733770921

I think I understand what you're trying to figure out, and I think the short answer is: this is only to indicate that such a parameter has a default value, which is impossible otherwise right now. Not really relevant for analysis.

But this is nice and simple and mirrors the native syntax precisely. :)

@ondrejmirtes
Copy link
Member

I agree. I just wanted to show the test with generics should be added in PHPStan to show we're actually reading the default value correctly. And if the class in new is actually generic, we should also resolve it correctly. So new Foo([1, 2, 3]) should result in correct Foo<T> same as in PHP code - we should probably construct PhpParser's New_ node and just ask Scope about it...

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs some changes in Printer too. At least definitely a one in $listInsertionMap and maybe more.

Not all entries in that property are tested, but if you comment ArrayShapeNode::class . '->items' => ', ', you'll see the kind of test I'd like to see. It's somewhere around $addItemsToArrayShape in PrinterTest. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants